Fix GH-19685: Segfault when bzip2 filter has invalid parameters#19702
Fix GH-19685: Segfault when bzip2 filter has invalid parameters#19702alexandre-daubois wants to merge 1 commit intophp:masterfrom
Conversation
alexandre-daubois
force-pushed
the
stream-append-bz2-failure
branch
from
e6d35d8 to
4c88e07
Compare
| zend_long blocks = zval_get_long(tmpzval); | ||
| if (blocks < 1 || blocks > 9) { | ||
| php_error_docref(NULL, E_WARNING, "Invalid parameter given for number of blocks to allocate (" ZEND_LONG_FMT ")", blocks); | ||
| pefree(data->strm.next_in, persistent); |
There was a problem hiding this comment.
nit: only if you want, on master only maybe, making a goto label to cover the 3 cases.
There was a problem hiding this comment.
As this is master now, probably a good idea to address this comment.
|
It should be ok in this case but breaking filter chain is sometimes not safe (might cause breaks) so I will need to review it properly - I should be properly looking to filters in 2 or 3 weeks time. |
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
So I just spent some time to getting to know bzip2 extension to see if I'm missing anything by any chance but I'm not sure I understand what the point of this PR is.
You claim (in the PR title) that this is fixes the segfault but from the stacktrace in #19685 (comment), this looks to me that the fix for that is in #19708 because it's segfaulting due to trying to use ops write on data stream (that has got write ops NULL). That bug GH-19685 is really just a duplicate of GH-19705 - the only difference is that it's using bzip2 filter which also as got some buffered rubbish to write. Basically if you changed the blocks to 4 and work to 0, it would still segfault so this really doesn't fix anything and it's completely unrelated to that bug. In fact this is not a bug at all but just a BC break because currently it's just warning and then using defaults. But you changed it to not use defaults and fails. This should really go through deprecation.
So at least you need to change the branch, titles and description and then add it to the deprecation RFC for 8.6.
|
This doesn't require an RFC anymore due to the policy change. Could you rebase this @alexandre-daubois ? |
alexandre-daubois
force-pushed
the
stream-append-bz2-failure
branch
from
4c88e07 to
4893048
Compare
alexandre-daubois
requested review from
Girgias,
SakiTakamachi,
TimWolla,
adoy,
arnaud-lb,
derickr,
dstogov,
kocsismate and
ndossche
as code owners
|
Needs to target master however :) |
alexandre-daubois
force-pushed
the
stream-append-bz2-failure
branch
from
4893048 to
f53c270
Compare
alexandre-daubois
force-pushed
the
stream-append-bz2-failure
branch
from
f53c270 to
db58e31
Compare
alexandre-daubois
force-pushed
the
stream-append-bz2-failure
branch
from
db58e31 to
82bef7d
Compare
|
Added the |
| . Fixed bug GH-19685 (Segfault when bzip2 filter has invalid parameters). | ||
| (alexandre-daubois) |
There was a problem hiding this comment.
Is this really fixing a bug tho?
5 participants
Fix #19685
When
blocksorworkhave invalid values, it currently let the function execution continue. Instead, we should free some resources and return null to avoid the segfault.Part of https://wiki.php.net/rfc/deprecations_php_8_6